fix(cache): don't FIFO-evict strongly-held coordination locks (session_locks) - #993
fix(cache): don't FIFO-evict strongly-held coordination locks (session_locks)#993jlucaso1 wants to merge 1 commit into
Conversation
PortableCache::insert_new evicted the capacity-FIFO head unconditionally, with no strong_count guard — unlike the sibling reclaim_init_lock / run_pending_tasks paths. session_locks is a capacity-only (10k, no TTL) Cache<String, Arc<Mutex>> whose values are handed out as clones held across a decrypt/encrypt. Evicting an address that is actively held lets the next session_lock_for miss and mint a second mutex, so two writers enter the same Signal ratchet concurrently -> counter/nonce reuse or SessionError. The FIFO victim is insertion-order, so a long-lived, actively-used address is the first to go. Add an opt-in eviction guard: insert_new skips capacity-eviction of entries the guard rejects, and overflows (bounded, transient) if none are evictable rather than dropping a live lock. Wire session_locks with `|lock| Arc::strong_count(lock) == 1`. Default (no guard) stays plain FIFO. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1geaAZffSxDhP7dpNrbbt
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughPortableCache gains an optional ChangesValue-aware eviction for PortableCache
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PortableCache
participant CacheInner
Client->>PortableCache: insert(session_key, lock)
PortableCache->>CacheInner: insert_new(evict_guard)
CacheInner->>CacheInner: check strong_count(lock) == 1 for oldest entries
alt unheld lock found
CacheInner->>CacheInner: evict unheld lock
else all locks held
CacheInner->>CacheInner: allow temporary overflow
end
CacheInner-->>Client: insertion complete
Possibly related PRs
Suggested labels: Listen, this isn't a small tweak, it's an infrastructure investment. We can't have mutexes getting evicted while they're doing real work — that's not how you build a platform that connects the world. So now, we check strong_count before we evict, like any serious engineering org would. Move fast, but don't break sessions. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/portable_cache.rs | Adds evict_guard to PortableCache and wires it through insert_new; logic is sound and tests cover both skip-and-evict and bounded-overflow paths. Minor: the in-function comment in insert_new is verbose and explains "what" as well as "why". |
| src/client/lifecycle.rs | Wires evict_guard on session_locks with the correct Arc::strong_count == 1 predicate. chat_lanes (also a coordination-lock cache holding Arc<Mutex<()>>) does not receive the same guard. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant A as Task A
participant B as Task B
participant Cache as session_locks cache
participant M1 as Mutex M1
A->>Cache: "get_with("addr") → Arc<M1>"
Cache-->>A: "M1 (strong_count=2)"
A->>M1: lock().await (holds across decrypt/encrypt)
Note over Cache: capacity reached, insert "addr2"
alt Before fix (no evict_guard)
Cache->>Cache: FIFO pop_first → evict "addr" (M1)
B->>Cache: get_with("addr") → miss
Cache->>Cache: "create fresh Arc<M2>"
Cache-->>B: "M2 (strong_count=2)"
B->>M2: lock().await
Note over A,B: A holds M1, B holds M2 — RACE on ratchet
else "After fix (evict_guard = strong_count == 1)"
Cache->>Cache: "find oldest where strong_count==1 → skip M1, evict next"
B->>Cache: get_with("addr") → hit M1
Cache-->>B: "M1 (strong_count=3)"
B->>M1: lock().await (waits for A)
Note over A,B: Serialised correctly
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant A as Task A
participant B as Task B
participant Cache as session_locks cache
participant M1 as Mutex M1
A->>Cache: "get_with("addr") → Arc<M1>"
Cache-->>A: "M1 (strong_count=2)"
A->>M1: lock().await (holds across decrypt/encrypt)
Note over Cache: capacity reached, insert "addr2"
alt Before fix (no evict_guard)
Cache->>Cache: FIFO pop_first → evict "addr" (M1)
B->>Cache: get_with("addr") → miss
Cache->>Cache: "create fresh Arc<M2>"
Cache-->>B: "M2 (strong_count=2)"
B->>M2: lock().await
Note over A,B: A holds M1, B holds M2 — RACE on ratchet
else "After fix (evict_guard = strong_count == 1)"
Cache->>Cache: "find oldest where strong_count==1 → skip M1, evict next"
B->>Cache: get_with("addr") → hit M1
Cache-->>B: "M1 (strong_count=3)"
B->>M1: lock().await (waits for A)
Note over A,B: Serialised correctly
end
Comments Outside Diff (1)
-
src/client/lifecycle.rs, line 168-170 (link)chat_lanesstores aChatLanewhich itself containsenqueue_lock: Arc<async_lock::Mutex<()>>— structurally identical to thesession_locksvalue type. If a task holds a clone of aChatLane'senqueue_lockacross an.awaitand that lane is FIFO-evicted, the next message for the same chat mints a freshChatLaneand bypasses the serialisation lock, the same race this PR fixes forsession_locks. Waschat_lanesintentionally excluded from the evict guard (e.g. covered by a separate PR), or should it receive the same guard?Prompt To Fix With AI
This is a comment left during a code review. Path: src/client/lifecycle.rs Line: 168-170 Comment: `chat_lanes` stores a `ChatLane` which itself contains `enqueue_lock: Arc<async_lock::Mutex<()>>` — structurally identical to the `session_locks` value type. If a task holds a clone of a `ChatLane`'s `enqueue_lock` across an `.await` and that lane is FIFO-evicted, the next message for the same chat mints a fresh `ChatLane` and bypasses the serialisation lock, the same race this PR fixes for `session_locks`. Was `chat_lanes` intentionally excluded from the evict guard (e.g. covered by a separate PR), or should it receive the same guard? How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/portable_cache.rs:100-107
The 6-line block comment inside `insert_new` narrates the algorithm step-by-step ("Pick the oldest entry…", "Without a guard that is simply the FIFO head…"), which violates the project's style guide (AGENTS.md): comments should only explain *why*, not *what*, and should be kept concise. The same verbosity appears in the test comment (`// F6: a coordination-lock cache must not FIFO-evict…`) and in the two-line comment added to `lifecycle.rs`. A single "why" sentence at the call-site is sufficient; the mechanics are evident from the code.
```suggestion
// With a guard, skip entries the guard disallows; if none are evictable,
// overshoot capacity until the held entries are released.
let victim = match evict_guard {
None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())),
Some(guard) => self
.order
.iter()
.find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value)))
.map(|(&seq, k)| (seq, k.clone())),
};
```
### Issue 2 of 2
src/client/lifecycle.rs:168-170
`chat_lanes` stores a `ChatLane` which itself contains `enqueue_lock: Arc<async_lock::Mutex<()>>` — structurally identical to the `session_locks` value type. If a task holds a clone of a `ChatLane`'s `enqueue_lock` across an `.await` and that lane is FIFO-evicted, the next message for the same chat mints a fresh `ChatLane` and bypasses the serialisation lock, the same race this PR fixes for `session_locks`. Was `chat_lanes` intentionally excluded from the evict guard (e.g. covered by a separate PR), or should it receive the same guard?
Reviews (1): Last reviewed commit: "fix(cache): don't FIFO-evict strongly-he..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let victim = match evict_guard { | ||
| None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())), | ||
| Some(guard) => self | ||
| .order | ||
| .iter() | ||
| .find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value))) | ||
| .map(|(&seq, k)| (seq, k.clone())), | ||
| }; |
There was a problem hiding this comment.
insert_new narrates the algorithm step-by-step ("Pick the oldest entry…", "Without a guard that is simply the FIFO head…"), which violates the project's style guide (AGENTS.md): comments should only explain why, not what, and should be kept concise. The same verbosity appears in the test comment (// F6: a coordination-lock cache must not FIFO-evict…) and in the two-line comment added to lifecycle.rs. A single "why" sentence at the call-site is sufficient; the mechanics are evident from the code.
| let victim = match evict_guard { | |
| None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())), | |
| Some(guard) => self | |
| .order | |
| .iter() | |
| .find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value))) | |
| .map(|(&seq, k)| (seq, k.clone())), | |
| }; | |
| // With a guard, skip entries the guard disallows; if none are evictable, | |
| // overshoot capacity until the held entries are released. | |
| let victim = match evict_guard { | |
| None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())), | |
| Some(guard) => self | |
| .order | |
| .iter() | |
| .find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value))) | |
| .map(|(&seq, k)| (seq, k.clone())), | |
| }; |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/portable_cache.rs
Line: 100-107
Comment:
The 6-line block comment inside `insert_new` narrates the algorithm step-by-step ("Pick the oldest entry…", "Without a guard that is simply the FIFO head…"), which violates the project's style guide (AGENTS.md): comments should only explain *why*, not *what*, and should be kept concise. The same verbosity appears in the test comment (`// F6: a coordination-lock cache must not FIFO-evict…`) and in the two-line comment added to `lifecycle.rs`. A single "why" sentence at the call-site is sufficient; the mechanics are evident from the code.
```suggestion
// With a guard, skip entries the guard disallows; if none are evictable,
// overshoot capacity until the held entries are released.
let victim = match evict_guard {
None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())),
Some(guard) => self
.order
.iter()
.find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value)))
.map(|(&seq, k)| (seq, k.clone())),
};
```
**Context Used:** AGENTS.md ([source](https://app.greptile.com/oxidezap/github/oxidezap/whatsapp-rust/-/custom-context?memory=26029e85-0dae-44f2-ab23-b8de43e5e9c7))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
What
Give
PortableCachean opt-in eviction guard so a capacity-only coordination-lock cache never FIFO-evicts an entry that a task still holds, and wiresession_lockswith|lock| Arc::strong_count(lock) == 1.Why (bug)
CacheInner::insert_newevicted the capacity-FIFO head unconditionally — nostrong_countguard, unlike the siblingreclaim_init_lock(strong_count <= 2) andrun_pending_tasks(retain strong_count > 1) in the same file.session_locksis a capacity-only (10_000, no TTL)Cache<String, Arc<async_lock::Mutex<()>>>.session_lock_forhands out clones of theArc, held across a Signal decrypt/encrypt. Eviction is pure insertion-order FIFO (getnever refreshes ordering), so a long-lived, actively-used address is the first victim. If task A holds mutexM1across an.awaitinsidemessage_decryptand A's address is FIFO-evicted, task B'ssession_lock_formisses, mints a freshM2 ≠ M1, and enters the ratchet for the same session concurrently → counter/nonce reuse on encrypt orSessionErroron decrypt. The lifecycle comment already flags exactly this hazard for time-based eviction ("would silently break serialisation") — capacity eviction had the same flaw, only mitigated by sizing.This is the same locking-discipline family as the already-merged #990 (send fan-out) and #992 (group inbound); this closes the lock-cache side.
How
insert_newgains an optionalevict_guard: Option<&dyn Fn(&V) -> bool>. When set, it evicts the oldest entry the guard allows, skipping still-referenced entries. If nothing is evictable it leaves the map to exceed capacity — a bounded, transient overshoot (held locks are released at the end of each critical section; concurrency is capped by the processing semaphore) that self-corrects on the next insert. Without a guard, behavior is unchanged plain FIFO.PortableCacheBuilder::evict_guard(f)wires it;session_locksusesArc::strong_count(lock) == 1.Tests
test_evict_guard_skips_strongly_held_entries— a held (strong_count > 1) FIFO head is skipped and the next unheld entry is evicted instead.test_evict_guard_overflows_when_all_held_then_recovers— all-held → bounded overflow (no live lock dropped); once released, a later insert evicts back to capacity.test_capacity_eviction,test_session_lock_pattern, and the FIFO-order tests still pass (default path unchanged).cargo fmt/clippy -p whatsapp-rust --lib --testsclean.🤖 Generated with Claude Code
Generated by Claude Code